Skip to content

test(convert): add tests for convert_to_json - #711

Merged
andrewtavis merged 4 commits into
scribe-org:mainfrom
GonzaFP:test_cli_convert_wrapper_py
Sep 20, 2026
Merged

andrewtavis merged 4 commits into
scribe-org:mainfrom
GonzaFP:test_cli_convert_wrapper_py

Conversation

@GonzaFP

@GonzaFP GonzaFP commented Aug 10, 2026 •

Copy link
Copy Markdown
Contributor

Contributor checklist


Description

Added tests for convert_to_json in test/cli/convert/wrapper.py to improve test coverage.

Related issue

@andrewtavis
andrewtavis self-requested a review August 12, 2026 05:37
@andrewtavis

Copy link
Copy Markdown
Member

Thanks for the PR, @GonzaFP! We'll try to get to the review soon :)

@andrewtavis

Copy link
Copy Markdown
Member

Quick check in here, @GonzaFP 👋 Do we need to add all of these tests, or are these functionalities being covered by src/scribe_data/cli/convert/to_json.py? Just checking as by the looks of this this covers one extra line of code, and 120 lines of tests for covering one line of code is a bit much. Feel free to simplify them down to just calling the JSON conversion from the wrapper! Looks like we also need a similar test that we can do convert_to_csv_or_tsv from the wrapper and then that file would be 100%.

Happy to keep these changes if they're needed, but it'd be great if you could verify if some of the tests are repeated :)

@GonzaFP

GonzaFP commented Aug 15, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks @andrewtavis for your response.
In wrapper.py, there are two conditions at the start of the function convert_wrapper that check if the output_dir is None and if input_path is None. I wrote two tests to test the conditions.
The convert_to_json function takes a language parameter of type string so I tested that too.

Should I combine all three tests into one?
PS: The prior tests of convert_to_sqlite served as an inspiration. I see that separate tests were written for each scenario.
I will work on convert_to_csv_or_tsv after convert_to_json is finished.

@andrewtavis

Copy link
Copy Markdown
Member

@GonzaFP: Note that I do still have this PR in mind. I've been away over the weekend. The community will be switching over to Scribe-Data from Scribe-Android this coming weekend and we'll bring this in then 😊 Sorry for your wait!

@GonzaFP

GonzaFP commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Noted @andrewtavis

@andrewtavis andrewtavis added the no-changelog No changelog entry is needed for this pull request label Sep 20, 2026

@andrewtavis andrewtavis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Thanks for sending along these tests, @GonzaFP, and sorry for your wait in this being brought in. We'll be doing a refactor of the tests in the coming months once we're done with the main three issues. Work there will be in #715 :)

@andrewtavis
andrewtavis merged commit 1782dfa into scribe-org:main Sep 20, 2026
7 checks passed
@GonzaFP

GonzaFP commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @andrewtavis.
I will check out the other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog No changelog entry is needed for this pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants